Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Micrometer 1.13.x migration recipe #11

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

jjank
Copy link
Contributor

@jjank jjank commented Jul 12, 2024

What's changed?

Initial work on Migrate to micrometer v1.13.+

What's your motivation?

Anything in particular you'd like reviewers to focus on?

  • How should this be incorporated into the "main" upgrade recipe?
  • Does the build need changes (with regards to pinnig)
  • Are the tests good enough?

Anyone you would like to review specifically?

@timtebeek

Any additional context

The migration from io.prometheus.client.CollectorRegistry to io.prometheus.metrics.model.registry.PrometheusRegistry is probably not covering all cases.
But for many code bases simply changing the type might be sufficient because CollectorRegistry and PrometheusRegistry have a very similar (and small) public api that essentially only offers a singleton - exposed as constant with equal names - and a constructor

Checklist

  • [] I've added unit tests to cover both positive and negative cases (sort of - no negative cases 😢 )
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

The migration from `io.prometheus.client.CollectorRegistry` to
`io.prometheus.metrics.model.registry.PrometheusRegistry` is probably
not covering _all_ cases.
But for many code bases simply changing the type might be sufficient
because `CollectorRegistry` and `PrometheusRegistry` have a very similar
(and small) public api that essentially only offers a singleton - exposed
as constant with equal names - and a constructor

Partially resolves openrewrite#8
@timtebeek timtebeek self-requested a review July 12, 2024 09:06
jjank and others added 2 commits July 12, 2024 11:11
…st.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…st.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Great start here @jjank ; thanks for your continued support of the project. These changes look great to me; We can include them in the Spring Boot 3.3 migration in rewrite-spring already to ensure users are at least seeing a start to the required changes.

@jjank
Copy link
Contributor Author

jjank commented Jul 12, 2024

That makes sense (I see this is already refered to in openrewrite/rewrite-spring#526).

I'd be happy to contribute this particular recipe to the Boot 3.3 migration after it's released. At some point it probably should be incorporated into the "generic" org.openrewrite.micrometer.UpgradeMicrometer recipe.

@timtebeek
Copy link
Contributor

I've indeed linked the two right now ; in rewrite-spring I think it's best to include org.openrewrite.micrometer.UpgradeMicrometer13 for the Spring Boot 3.3 migration, as opposed to UpgradeMicrometer, such that a year from now that's not picking up a wildly different version just yet.

@timtebeek timtebeek merged commit 37b2aab into openrewrite:main Jul 12, 2024
2 checks passed
@jjank jjank deleted the 8-micrometer-13 branch July 12, 2024 09:32
@timtebeek
Copy link
Contributor

That makes sense (I see this is already refered to in openrewrite/rewrite-spring#526).

I'd be happy to contribute this particular recipe to the Boot 3.3 migration after it's released.

Great! The way our projects depend upon each other there's no need to wait for a release; rewrite-spring would be able to depend on this change already. Be sure to refresh your dependencies to pull down the latest snapshot from Sonatype OSS Snapshots, built from the main branch here.

We wouldn't need more than just an added dependency + inclusion of the recipe there; we already tested the changes here.

@jjank
Copy link
Contributor Author

jjank commented Jul 12, 2024

Cool, I'll give it a try right away 👍

kmccarp pushed a commit to kmccarp/rewrite-micrometer that referenced this pull request Aug 22, 2024
* Add Micrometer 1.13.x migration recipe

The migration from `io.prometheus.client.CollectorRegistry` to
`io.prometheus.metrics.model.registry.PrometheusRegistry` is probably
not covering _all_ cases.
But for many code bases simply changing the type might be sufficient
because `CollectorRegistry` and `PrometheusRegistry` have a very similar
(and small) public api that essentially only offers a singleton - exposed
as constant with equal names - and a constructor

Partially resolves openrewrite#8

* Update src/test/java/org/openrewrite/micrometer/UpgradeMicrometer13Test.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update src/test/java/org/openrewrite/micrometer/UpgradeMicrometer13Test.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Loop in 1.13.x upgrade with the existing UpgradeMicrometer

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tim te Beek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants